Introduced RequestSource metadata to distinguish between API and ruler queries #6947
Conversation
dosubot
bot
added
the
component/rules
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
5e39a8b to
53cedc7
Compare
erlan-z
changed the title
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
53cedc7 to
47da956
Compare
pkg/ingester/ingester.go
Outdated
| @@ -2262,7 +2263,7 @@ func (i *Ingester) trackInflightQueryRequest() (func(), error) { | |||
|
|
|||
| i.maxInflightQueryRequests.Track(i.inflightQueryRequests.Inc()) | |||
|
|
|||
| if i.resourceBasedLimiter != nil { | |||
| if i.resourceBasedLimiter != nil && requestmeta.RequestSourceFromContext(ctx) == requestmeta.SourceApi { | |||
There was a problem hiding this comment.
nit: what do you think of adding helper functions in requestmeta itself? ie. requestmeta.RequestFromApi(ctx) and requestmeta.RequestFromRuler(ctx)
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
47da956 to
8bb3d4f
Compare
pkg/util/requestmeta/source.go
Outdated
| const ( | ||
| SourceApi = "api" | ||
| SourceRuler = "ruler" | ||
| ) |
There was a problem hiding this comment.
How about reusing tripperware.SourceAPI and tripperware.SourceRuler?
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
bfdbdd3 to
7cfa79a
Compare
There was a problem hiding this comment.
As a feature that intends to protect storage layer, I am not sure if it is the best way to hardcode it by differentiating source of requests and only rejecting one type but not others. I understand the use case behind this change. But if we go with this, it opens a door that we may need to extend and support throttling based on other types of source such as user agent, dashboard ID, etc.
I think a better approach is to go with #6922 and throttle heavy queries no matter its source.
Thanks for the review. I agree #6922 is the right direction to avoid hard-coding on user attributes like User-Agent or dashboard ID. That said, I see ruler vs. ad-hoc queries as a different category: it’s a system-level distinction, whereas the others are user-driven. So I don’t think this opens the door to extending based on those other characteristics. I also recognize that ruler queries can sometimes be heavy, but that’s a rare corner case and not really the problem this feature is trying to solve. Because #6922 is more of a best-effort mechanism, I’d even suggest carrying this system-level distinction into that work as well, to avoid ruler queries being rejected in cases where precision isn’t perfect. What do you think? |
I don't think we can easily identify queries coming from API as ad-hoc queries. There are different usecases that might be even more important than rules and it is all up to the user to decide the priority. Even rules can come from the query API endpoint from some remote evaluators like Thanos Ruler. |
Makes sense, thanks. I’m aligned with #6922. I realize it’s not always clear which queries are higher priority, and there can certainly be important API queries too, but since we can’t reliably identify those, it might help to at least avoid rejecting ruler queries, which are usually the priority ones we can identify in practice. Happy to leave this out here and revisit if needed. |
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
7cfa79a to
c4cfec4
Compare
|
Udpated PR to only include change to add source for request, we can leave out resource based throttling for adhoc queries part for now. |
erlan-z
changed the title
…replace request source implementation on qfe when ruler calls qfe to unify experience across services Signed-off-by: Erlan Zholdubai uulu <[email protected]>
erlan-z
force-pushed
the
resource-based-throttling-reject-only-addhoc-queries
branch
from
c4cfec4 to
c87c478
Compare
…his shouldn't have been included as discussed in PR cortexproject#6947 Signed-off-by: Erlan Zholdubai uulu <[email protected]>
…his shouldn't have been included as discussed in PR #6947 (#7000) Signed-off-by: Erlan Zholdubai uulu <[email protected]>
* remove limiting query rejection to only adhoc queries for ingester. This shouldn't have been included as discussed in PR #6947 Signed-off-by: Erlan Zholdubai uulu <[email protected]> * bug fixes for query rejection. fix metric and remove unused enabled flag. Signed-off-by: Erlan Zholdubai uulu <[email protected]> --------- Signed-off-by: Erlan Zholdubai uulu <[email protected]>
What this PR does:
Introduced RequestSource metadata to distinguish between API and ruler queries
Description:
Introduced RequestSource metadata to distinguish between API and ruler queries and propagates through services.
We previously had this source parsed from the "User-Agent" header which was only available in QFE. To extend this to other services, and to have unified experience across services, this PR changes that to keep source in context similar to RequestId.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]